fix(codemod): keep request() result schemas the call still needs#2383
Conversation
🦋 Changeset detectedLatest commit: 0790655 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good — the spec-method guard and changeset wording from the earlier round are addressed in 67bfe3f; the remaining inline note about extending the carve-out to compat/union schemas (e.g. CompatibilityCallToolResultSchema) is a minor edge case, not a blocker.
Extended reasoning...
Overview
This PR is confined to the @modelcontextprotocol/codemod package: it tightens the v1→v2 schema-param-removal transform so request() only loses its result-schema argument when the method is a provably-literal spec method (and never when the schema is the generic passthrough ResultSchema), while callTool() behavior is unchanged. The diff also bumps the auto-generated versions.ts and adds a changeset plus nine new transform tests covering proxy/forwarder shapes, as const/template-literal unwrapping, spread-after-method, and the literal non-spec-method case.
Security risks
None. The change only affects a developer-run migration tool that rewrites source ASTs locally; it does not touch transports, auth, protocol handling, or any runtime SDK code path.
Level of scrutiny
Moderate. The transform logic involves AST analysis, but the change makes the codemod strictly more conservative — it rewrites fewer call sites, and every kept schema still compiles in v2. Failure modes are limited to suboptimal (but compiling) migrated code rather than runtime SDK breakage. The earlier review round's blocking issue (literal non-spec methods having their schema stripped) and the prose inaccuracy in the changeset are both addressed in the latest commit: the guard now checks SPEC_REQUEST_METHODS derived from the existing SCHEMA_TO_METHOD mapping, the changeset says schema-less request() throws a TypeError for non-spec methods, and a test pins the literal non-spec-method case.
Other factors
The remaining finding posted inline — that the ResultSchema-only carve-out doesn't extend to other broader-than-spec schemas like CompatibilityCallToolResultSchema — is a genuine but narrow edge case (explicit compat schema on a literal spec-method call talking to a 2024-10-07 server), surfaces as a clear runtime validation error rather than silent corruption, and was already the pre-existing behavior before this PR. Test coverage is thorough (399 codemod tests passing per the description), the changeset is a patch bump for the codemod only, and no public SDK API surface changes.
The schema-param-removal rule dropped the result-schema argument from every MCP-schema-bearing request()/callTool() call. For request() that is only safe when the method is provably a literal string: schema-less v2 request() enforces the spec result schema for known methods and resolves undefined for unknown ones, so stripping the schema from a dynamic-method call site (the proxy/forwarder shape) silently changed forwarding semantics and broke verbatim relays. request() now keeps its schema unless the request object literal carries a literal method (string or no-substitution template, unwrapping as-const/ satisfies/parens, and not followed by a spread that could override it), and the generic passthrough ResultSchema is always kept. callTool() is unchanged: its v2 signature has no schema parameter, so the argument always goes.
Review follow-up: a literal method proves nothing about spec-ness, and
schema-less v2 request() throws a TypeError for non-spec methods, so a
custom-method call site like request({ method: 'acme/search' }, Schema)
lost a schema the call still needs. The drop is now gated on the literal
appearing in the spec request-method set (SCHEMA_TO_METHOD values). Also
corrects the changeset and comments: unknown methods fail loudly with a
TypeError, they do not resolve undefined.
felixweinberger
force-pushed
the
fweinberger/codemod-keep-dynamic-result-schema
branch
from
67bfe3f to
0790655
Compare
felixweinberger
deleted the
fweinberger/codemod-keep-dynamic-result-schema
branch
1 participant
The v1→v2 codemod's schema-param-removal rule dropped the result-schema argument from every
request()/callTool()call whose second argument was an MCP-imported*Schema. Forrequest()that is only safe when the method is provably a literal: schema-less v2request()enforces the spec result schema for known methods and resolvesundefinedfor unknown ones, so stripping the schema from a dynamic-method call site — the proxy/forwarder shape,request({ method, params }, ResultSchema)— silently changed forwarding semantics and broke verbatim relays.Motivation and Context
Found by dry-running the migration on a real multi-tenant relay: 12+ test failures traced to this rewrite (non-conforming upstream results rejected locally, unknown-method results becoming
undefined, conforming results re-serialized in schema key order). The migration guide's new gateway/proxy section (#2382) documents the workaround for already-migrated code; this fixes the cause.request()now keeps its schema unless the request object literal carries a literalmethod(string or no-substitution template, unwrappingas const/satisfies/ parentheses, and not followed by a spread that could override it at runtime). The generic passthroughResultSchemais always kept — even for literal spec methods, dropping it would switch the call from passthrough validation to spec-schema enforcement.callTool()is unchanged: its v2 signature has no schema parameter, so the argument always goes.How Has This Been Tested?
Nine new transform tests covering the proxy/forwarder shapes (shorthand dynamic method, variable method property, non-object-literal request, generic and aliased
ResultSchema,as const, spread-after-method) and pins for the unchanged behavior (literal methods still drop,callTool()always drops). Full codemod suite: 399 passing. Typecheck and lint clean.Breaking Changes
None — the codemod now rewrites strictly fewer call sites; the kept schemas compile in v2 (
request()accepts any Standard Schema and the import-path transform routes the constants to@modelcontextprotocol/core).Types of changes
Checklist
Additional context
Once this publishes, the guide sentence added in #2382 ("the codemod may have dropped the schema argument there; restore it") can be softened to reference codemod versions before this fix.